-
-
Notifications
You must be signed in to change notification settings - Fork 194
Child process queue workers #450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Remove `ShouldBroadcast` in favor of `ShouldBroadcastNow` in `Events\\ChildProcess` namespace - Implement `QueueWorker::class` - Add new binding to `NativeServiceProvider::class` - Fire up workers: iterate through queue worker config in `NativeServiceProvider::configureApp()` - Test `QueueWorker::up()`, `QueueWorker::down()` - Test `QueueWorkerFake::class` assertions work as expected
That was on my list of suggestions and changes I would like to see in Native PHP. A quick look at it and everything seems fine to me. I have a small suggestion to improve the developer experience for newcomers. We should add the command to the // Launch the queue worker
QueueWorker::up() Also a PR to update electron and another for the documentation. When the PR for |
Electron PR is up: NativePHP/electron#149
// NativeServiceProvider::class
protected function fireUpQueueWorkers(): void
{
$queueConfigs = QueueConfig::fromConfigArray(config('nativephp.queue_workers'));
foreach ($queueConfigs as $queueConfig) {
$this->app->make(QueueWorkerContract::class)->up($queueConfig);
}
} Adding Whether we should do this in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this looks great 👍🏼 I think it's right that we provide this functionality internally by default to save the developer having to think about setting workers up.
We should just make sure this works even when there is no queue_workers
key in the config file (which is the state most apps will be in right now.
Once the docs are updated, I think this will be ready to go.
Great work @XbNz 🙏🏼
Documentation updated NativePHP/nativephp.com#69 |
Rely on keys of the config array to assert uniqueness of worker aliases
Commits related to feature discussed in #443
Note
nativephp/electron
must be updated to remove javascript-side queue systemRemove
ShouldBroadcast
in favor ofShouldBroadcastNow
inEvents\\ChildProcess
namespaceImplement
QueueWorker::class
Add new binding to
NativeServiceProvider::class
Fire up workers: iterate through queue worker config in
NativeServiceProvider::configureApp()
Test
QueueWorker::up()
,QueueWorker::down()
Test
QueueWorkerFake::class
assertions work as expected